Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

add initial env var draft specification #666

Merged
merged 15 commits into from
Aug 5, 2020
Merged

add initial env var draft specification #666

merged 15 commits into from
Aug 5, 2020

Conversation

mwear
Copy link
Member

@mwear mwear commented Jun 23, 2020

Fixes #572.

This is an initial draft of an environment variable specification. As stated in the spec, the goal is to unify the names of environment variables between SDK implementations. SDKs are not required to add configuration for the variables specified, but if they do, they should align with names in this spec.

This list was compiled from comments on #572 and surveying a number of SDK implementations. Defaults have been provided where applicable and likely to be uncontroversial. The door is open to add more detail if we would like to.

Lastly, this is meant to be a starting to point document and align environment variable names going forward. To that end, if a SIG adds a new environment variable that is applicable across languages, it should be documented here. On the same note, if there are environment variables that are more widely applicable that were missed during the survey, add them in the comments and I'll update the PR.

@mwear mwear force-pushed the env_var_spec branch 2 times, most recently from af73309 to 77a2598 Compare June 24, 2020 03:37
codeboten
codeboten reviewed Jun 24, 2020
View reviewed changes
Copy link
Contributor

@codeboten codeboten left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like a good starting point, some comments.

specification/sdk-environment-variables.md Outdated Show resolved Hide resolved
specification/sdk-environment-variables.md Outdated Show resolved Hide resolved
specification/sdk-environment-variables.md Outdated Show resolved Hide resolved
@mwear mwear requested a review from a team June 24, 2020 17:39
@carlosalberto
Copy link
Contributor

Have you considered adding a note on OTel language-specific configuration options? Probably mention they are expected to use the same prefix? i.e. OTEL_[LANGUAGE]_[FEATURE] or similar?

Overall looks good, great work!

@malafeev
Copy link
Contributor

In opentelemetry-java PR I proposed more environment variables for OTLP Exporter:

 *   OTEL_OTLP_ENDPOINT - to set the endpoint to connect to.
 *   OTEL_OTLP_USE_TLS -  to set use or not TLS.
 *   OTEL_OTLP_METADATA - to set key-value pairs separated by semicolon to pass as request metadata.

@malafeev malafeev mentioned this pull request Jun 25, 2020
Merged
@james-bebbington james-bebbington mentioned this pull request Jun 29, 2020
Merged
This was referenced Jun 30, 2020
Open
Closed
Closed
Closed
Merged
@carlosalberto carlosalberto added spec:miscellaneous For issues that don't match any other spec label release:required-for-ga Must be resolved before GA release, or nice to have before GA labels Jul 10, 2020
codeboten
codeboten approved these changes Jul 13, 2020
View reviewed changes
Copy link
Contributor

@codeboten codeboten left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a good starting point, we can add more variables later but we should have a starting point.

@codeboten codeboten mentioned this pull request Jul 13, 2020
Merged
@trask trask mentioned this pull request Jul 16, 2020
Closed
@iNikem
Copy link
Contributor

iNikem commented Jul 17, 2020

@open-telemetry/specs-approvers This PR documents the current state of implementation. When merged, it will provide a good place for submitting PRs about improving environment variable recommendations. I don't see much controversy during the discussion of this PR, so it seems that we can approve and merge it.

This was referenced Jul 17, 2020
Closed
Closed
@carlosalberto carlosalberto self-requested a review July 17, 2020 13:40
@carlosalberto
Copy link
Contributor

Approving. A few of the items that will need further refinement (sampling, propagators) already have a related issue, so we can iterate on them after this PR gets merged.

@iNikem
Copy link
Contributor

iNikem commented Jul 17, 2020

@carlosalberto Anything else is needed before this PR can be merged?

@carlosalberto
Copy link
Contributor

Oops, wrong PR comment.

For this PR we need at least one approval. @arminru @yurishkuro could be interesting in reviewing this? ;)

yurishkuro
yurishkuro reviewed Jul 17, 2020
View reviewed changes
specification/sdk-environment-variables.md Outdated Show resolved Hide resolved
specification/sdk-environment-variables.md Outdated Show resolved Hide resolved
specification/sdk-environment-variables.md Outdated Show resolved Hide resolved
specification/sdk-environment-variables.md Outdated Show resolved Hide resolved
specification/sdk-environment-variables.md Outdated Show resolved Hide resolved
@iNikem
Copy link
Contributor

iNikem commented Jul 22, 2020

@yurishkuro do you approve this PR now? Anything else should be done here before we can merge it?

@trask trask mentioned this pull request Jul 22, 2020
Merged
@carlosalberto
Copy link
Contributor

Ping @yurishkuro

I think all your issues have been solved ;)

@carlosalberto
Copy link
Contributor

@open-telemetry/specs-approvers Please review. We have had a few iterations so far, and all things have been addressed (and for the things that are not trivial, there are new, related tickets). For your consideration ;)

@carlosalberto
Copy link
Contributor

Given that the feedback has been addressed, follow-up issues have been filled, and we have a few approvals (even if not all come from OTel approvers), I will merge this by tomorrow, FYI @open-telemetry/specs-approvers

@jtescher jtescher mentioned this pull request Aug 4, 2020
Closed
@carlosalberto carlosalberto merged commit 3fce938 into open-telemetry:master Aug 5, 2020
@carlosalberto carlosalberto mentioned this pull request Aug 5, 2020
Merged
@jkwatson jkwatson mentioned this pull request Aug 5, 2020
Closed
carlosalberto pushed a commit to carlosalberto/opentelemetry-specification that referenced this pull request Oct 31, 2024
@mwear
add initial env var draft specification (open-telemetry#666)
41d50f2
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yurishkuro yurishkuro yurishkuro left review comments

@tsloughter tsloughter tsloughter left review comments

@iNikem iNikem iNikem approved these changes

@carlosalberto carlosalberto carlosalberto approved these changes

@codeboten codeboten codeboten approved these changes

@anuraaga anuraaga anuraaga approved these changes

Assignees
No one assigned
Labels
release:required-for-ga Must be resolved before GA release, or nice to have before GA spec:miscellaneous For issues that don't match any other spec label
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Standardize environment variables between SDK implementations
8 participants
@mwear @carlosalberto @malafeev @iNikem @tsloughter @anuraaga @codeboten @yurishkuro